-
Notifications
You must be signed in to change notification settings - Fork 789
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix setup issues for Roslyn insertion #1657
Conversation
@@ -59,7 +59,4 @@ | |||
<Asset Type="Microsoft.VisualStudio.MefComponent" d:Source="Project" d:ProjectName="FSharp.Editor" Path="|FSharp.Editor|" AssemblyName="|FSharp.Editor;AssemblyName|" /> | |||
<Asset Type="Microsoft.VisualStudio.Analyzer" d:Source="Project" d:ProjectName="FSharp.Editor" Path="|FSharp.Editor|" /> | |||
</Assets> | |||
<Prerequisites> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we removing these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was needed for a local setup issue by Kevin, but it is failing building in willow for now. Discussed it with @KevinRansom we removed the dependency as it is not required ATM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These will be required to build with RC builds of VS "15" btw.
<package id="WiX.Toolset.2015" version="3.10.0.1503" /> | ||
<package id="Microsoft.VisualFSharp.Core.Redist" version="1.0.0" /> | ||
<package id="Microsoft.VisualStudio.Shell.14.0" version="14.3.25407" targetFramework="net46" /> | ||
<package id="Microsoft.VisualFSharp.Msbuild.15.0" version="1.0.1" /> | ||
<package id="System.ValueTuple" version="4.0.0-rc3-24212-01" /> | ||
<package id="Microsoft.VisualFSharp.Type.Providers.Redist" version="1.0.0" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see this as part of the change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this because you moved it here from vs-integration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved. These are not needed in vsintegration. Only used in setup.
<bindingRedirect | ||
oldVersion="2.0.0.0-4.4.1.0" | ||
newVersion="4.0.0.0"/> | ||
</dependentAssembly> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this related to the bug above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Needed so that NGEN can load the correct dependencies.
publicKeyToken="b03f5f7f11d50a3a" | ||
culture="neutral"/> | ||
<bindingRedirect | ||
oldVersion="2.0.0.0-4.4.1.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think value tuple needs this binding redirect. In the future it may need one. but not right now.
@@ -12,6 +12,15 @@ | |||
oldVersion="2.0.0.0-4.4.1.0" | |||
newVersion="4.4.1.0"/> | |||
</dependentAssembly> | |||
<dependentAssembly> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Value tuple shouldn't need a binding redirect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@KevinRansom We are referencing version 4.0.0.0, so fsc shouldn't be affected by this change. BUT this is needed for NGEN. Do you have concerns for adding this redirection here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed offline. We can get rid of the dependency.
* Remove prequesities from vsix manifests * Fix dotnet#1655 - Upgrade to SwixBuild plugin version 1.0.71 * Fix dotnet#1654 - FSharp Binaries installed reference wrong versions * Bring back language service registration * Remove System.ValueTuple binding redirection
Fixes #1655
Fixes #1654
@Microsoft/fsharp-compiler